Un-stale Tasks 3-7 of the Martyrology OIDC plan - #25
Conversation
Two parts, both stemming from the same finding: cdcf-infra's copy of the LiturgicalCalendar authorization model is two revisions behind the deployed one, and setup-openfga.sh would upload it over the top. Protective docs (accurate until the design lands): - auth/handoffs/liturgicalcalendar.md — record the deployed model 01KW4FW2ZCT1E693PY8D9TJEFM and its real shape (test_definition and deleter are gone; general_roman_calendar, three *_test types and member_nation are new), correct the sample OPENFGA_MODEL_ID, and warn that --create-litcal-store must not be re-run until the file is synced. The Zitadel half of that handoff was re-verified against production and is accurate as recorded. - docs/SYSADMIN.md — same warning inline next to the command in 4.8, plus 5.4 and the component inventory noting the LitCal repo owns that model today. Design doc (docs/superpowers/specs/2026-08-04-openfga-model-ownership- and-upgrade-design.md): - Decision: centralize every model in cdcf-infra; project repos hold no model file and consume via the authz-seed pattern already working in martyrology-api. - LitCal migration in two PRs, cdcf-infra first. - Guardrail: per-store lock file recording the model ID this repo last uploaded, so an out-of-band upload makes the script refuse rather than silently regress a store. - OpenFGA 1.15.1 -> 1.18.2 is migration-free on Postgres (assets 001-006 identical at both tags); the v1.18.0 warning is MySQL-only. Includes the three image assumptions to verify before the production pin moves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two plans from the 2026-08-04 design, independent except that ownership Task 6 (the LitCal PR) needs ownership Task 5 merged first. 2026-08-04-openfga-model-ownership.md — sync the LitCal model, add the lock-file guard with a reproduce-then-fix cycle against a throwaway in-memory OpenFGA, generate lock files from the deployed IDs, update the docs, then the two PRs and a read-only production verification. 2026-08-04-openfga-1182-upgrade.md — verify the three image assumptions first (healthcheck binary, playground/preshared behaviour, and a v1.18.2 migrate run against a v1.15.1 database to confirm the no-op), then local stacks, then production with health, model-listing and Check probes plus a rollback that needs no schema work. Scoping note found while planning: the LitCal repo owns more than a model file. scripts/setup-openfga.sh creates the store, uploads the model, and its --update-env writes IDs into env files that the ops runbook and infrastructure README depend on, so the plan reduces that script to ID discovery plus env wiring rather than deleting it. The Frontend repo has no model copy, contrary to that repo's 2026-06-20 spec. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR #23 made --provision-martyrology-frontend target-aware, and the local dev stack (martyrology-api / martyrology-frontend feat/local-dev-stack, unmerged) now provisions a localhost Zitadel client in its own instance. Replace the "stale pending local-stack design" notice with the resolved guidance: verify Tasks 3, 5, 6 and 7's live steps against the local stack, one app per Zitadel instance, production untouched.
Review found the notice's own step bodies contradicted it: Task 3 Step 9 sent readers to nonexistent "Dev app values from Task 2" and created a `.env.local` Next.js never needed; Task 7's handoff template and issue-close comment still claimed two apps in one Zitadel project. Point Step 9 (and its Task 3 Step 6c / Task 5 Step 8 cross-references) at the local stack's `.env`, written by `setup-stack.sh --update-env`, and rewrite Task 7 Steps 1, 3 and 5 to the one-app-per-instance reality: one production app recorded in the handoff, a separately-provisioned local app in the local stack's own Zitadel instance.
Line 7 said "Two confidential OIDC Web apps are provisioned in the existing MartyrologyAPI Zitadel project" — the same two-apps-in-one-project model the notice two sections down now warns against, missed by an exact-phrase grep in the prior round. State one app per instance instead.
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR updates Martyrology OIDC provisioning instructions and adds OpenFGA model ownership, upgrade, synchronization, lock-file, deployment, and verification plans. It also records current LitCal production model details and stale-model warnings. ChangesMartyrology OIDC provisioning
OpenFGA ownership and upgrade
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@auth/handoffs/liturgicalcalendar.md`:
- Around line 41-43: Update the warning in the handoff text to say that checking
deleter or test_definition is invalid only against the current/latest model.
Preserve the surrounding history and the statement that existing pinned
consumers remain unaffected.
In `@docs/superpowers/plans/2026-08-03-martyrology-oidc-login-client.md`:
- Around line 398-429: Rename the heading “Tasks 3-7: local verification now
runs against the local stack” to indicate only Tasks 3, 5, 6, and 7 are
affected, such as “Tasks 3, 5-7: local verification now runs against the local
stack.” Leave the clarification bullets unchanged.
- Line 1637: Update the “Placeholder scan” statement to classify AUTH_ZITADEL_ID
as a runtime value rather than a runtime secret, while continuing to identify
AUTH_ZITADEL_SECRET and AUTH_SECRET as secrets.
- Around line 775-778: Update the test instructions around the imported module
to explicitly name the exact required AUTH_* environment variables, resolving
the mismatch between “four” and the five variables listed in the local-stack
setup: AUTH_ZITADEL_ISSUER, AUTH_ZITADEL_ID, AUTH_ZITADEL_SECRET, AUTH_SECRET,
and AUTH_URL. Correct the stated count if all five are required.
- Around line 832-842: Update the local-stack instructions around “docker
compose stop martyrology-frontend” to provide the correct working directory or
explicit Compose file defining that service. Use the repository’s actual
local-stack configuration, or remove this stop command if the stack no longer
includes martyrology-frontend.
In `@docs/superpowers/plans/2026-08-04-openfga-1182-upgrade.md`:
- Around line 371-385: Update Step 4’s data probes to issue an authorization
Check against the LiturgicalCalendar store as well as Martyrology. Add the
known-good LitCal tuple and verify its expected allowed decision, while
preserving the existing model-ID validation and Martyrology Check.
- Around line 70-83: Update the Step 4 Playground verification command to
guarantee termination in both startup-success and startup-failure cases, using
either a bounded detached health check with explicit container cleanup or a
timeout that cleans up the container afterward. Preserve the existing
preshared-auth and Playground environment settings and continue capturing enough
startup output to verify the expected error behavior.
- Around line 209-215: Update the migration steps in both the local verification
section and the production task to wait for openfga-migrate to finish before
starting openfga. Replace detached migration startup with exit-code-aware
execution or explicitly wait for and validate the one-shot container exits with
code 0, then run docker compose up -d openfga.
- Around line 161-167: Update Step 5’s pytest command so host tests are not used
when container tests fail: only fall back to the repository’s documented host
invocation when the API container is unavailable, while preserving a failing
status for container test failures. Keep a single canonical test-environment
path and retain the expected-pass instruction.
In `@docs/superpowers/plans/2026-08-04-openfga-model-ownership.md`:
- Around line 355-370: Update Step 10 to recreate the throwaway store’s
out-of-band state after the guard implementation, either by resetting the store
or re-uploading the Step 3 out-of-band model before running the dangerous
command. Ensure the verification then exercises the no-lock refusal branch and
asserts that the latest model ID remains unchanged.
- Around line 422-455: Update Task 3 Step 1 to fetch each deployed model’s full
content, normalize it using the same normalization as the lock-validation guard,
and compare it with the corresponding committed model file before creating
locks. Stop and report a mismatch instead of writing a lock when IDs or
normalized contents differ; only generate the lock files after both production
models match their repository definitions.
- Around line 341-345: Fix the documentation lint issues in the affected
Markdown sections: add an appropriate language identifier to the fence
containing --force-model-upload, mark the non-code fence with no-code, remove
spacing inside the nested code span, and change the relevant terminology to
end-to-end. Preserve the surrounding documentation content.
- Around line 766-781: Update the Step 2 verification script so a lock/live
model mismatch sets a failure flag instead of relying on the `&& ... || ...`
expression’s exit status. Initialize the flag before the `for` loop, set it when
printing `MISMATCH`, and exit non-zero after the loop if any mismatch occurred;
preserve the existing `OK` output and comparison behavior.
- Around line 136-143: Bind each throwaway OpenFGA Docker port to loopback by
updating the published mappings at
docs/superpowers/plans/2026-08-04-openfga-model-ownership.md lines 136-143,
docs/superpowers/plans/2026-08-04-openfga-1182-upgrade.md lines 58-65, and
docs/superpowers/plans/2026-08-04-openfga-1182-upgrade.md lines 74-80 to use
127.0.0.1 before the host ports, preserving the existing port numbers and
container port.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9661956a-f12f-410b-b1db-e468abdda62d
📒 Files selected for processing (6)
auth/handoffs/liturgicalcalendar.mddocs/SYSADMIN.mddocs/superpowers/plans/2026-08-03-martyrology-oidc-login-client.mddocs/superpowers/plans/2026-08-04-openfga-1182-upgrade.mddocs/superpowers/plans/2026-08-04-openfga-model-ownership.mddocs/superpowers/specs/2026-08-04-openfga-model-ownership-and-upgrade-design.md
| ``` | ||
| --force-model-upload Upload the model file even when the store's latest model | ||
| is not the one recorded in auth/models/NAME.lock.json | ||
| (i.e. someone uploaded out-of-band). Off by default. | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the reported documentation lint errors.
- Add a language to the fence at Line 341.
- Use
no-codeat Line 494. - Remove the nested code-span spacing at Line 514.
- Use
end-to-endat Line 711.
Also applies to: 494-494, 514-518, 711-711
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 341-341: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-08-04-openfga-model-ownership.md` around lines
341 - 345, Fix the documentation lint issues in the affected Markdown sections:
add an appropriate language identifier to the fence containing
--force-model-upload, mark the non-code fence with no-code, remove spacing
inside the nested code span, and change the relevant terminology to end-to-end.
Preserve the surrounding documentation content.
Source: Linters/SAST tools
Fixes across the OIDC login-client plan, both OpenFGA plan documents, and the LiturgicalCalendar handoff: correct scope/count claims, bind throwaway OpenFGA ports to 127.0.0.1, make the Playground startup probe terminate in both outcomes, stop pytest's container fallback from masking real failures, wait for openfga-migrate to exit 0 before starting openfga, add a LiturgicalCalendar Check probe alongside the Martyrology one, fix the Step 10 out-of-band regression test whose fixture state was already consumed by Step 3, verify deployed model content (not just ID) before writing a lock, fix a verification script whose &&/|| chain always exited 0, and clean up markdown lint issues.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-08-04-openfga-1182-upgrade.md`:
- Around line 370-375: Update the production migration validation around
MIGRATE_EXIT and the openfga-migrate logs to assert that the migration completed
successfully without applying changes before starting OpenFGA. Check the
captured docker compose log output for the expected no-op indicator, or compare
migration state before and after, and fail with a clear error if changes were
applied.
- Around line 230-232: Update both detached migration completion paths at
docs/superpowers/plans/2026-08-04-openfga-1182-upgrade.md lines 230-232 and
370-372 to use a reliable exited-container lookup before docker wait, such as
docker compose ps -q --status=exited or docker compose ps -a --status=exited, or
replace the flow with up --exit-code-from openfga-migrate; apply the same
approach to both local and production migration checks.
In `@docs/superpowers/plans/2026-08-04-openfga-model-ownership.md`:
- Around line 357-371: The Step 3 scenario is mislabeled as adoption and does
not validate that the out-of-band upload succeeded. Rename the relevant
description and assertion labels to “no-lock refusal branch,” and update the
upload commands to use curl’s HTTP-failure behavior plus validation that
OOB_MODEL is non-empty and not null before proceeding.
- Around line 475-486: Update the verification loop around the deployed model
response so it extracts both the model ID and normalized content from the same
response, rather than discarding the ID. Compare the extracted ID with the
expected ID from Step 1, and only report the model as safe and permit Step 3 to
write that verified ID when both the ID and content match; otherwise report a
mismatch and do not lock.
- Around line 819-830: The verification loop must reject missing lock fields and
failed API responses instead of allowing null values to compare equal. Update
the jq lookups in the LiturgicalCalendar/Martyrology loop to use jq -er with //
empty, add curl --fail-with-body to the live-model request, and mark any empty
store, model, or live value as a verification failure before comparing IDs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fecc9acd-9ed6-4364-8135-f99135102786
📒 Files selected for processing (4)
auth/handoffs/liturgicalcalendar.mddocs/superpowers/plans/2026-08-03-martyrology-oidc-login-client.mddocs/superpowers/plans/2026-08-04-openfga-1182-upgrade.mddocs/superpowers/plans/2026-08-04-openfga-model-ownership.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/superpowers/plans/2026-08-03-martyrology-oidc-login-client.md
- auth/handoffs/liturgicalcalendar.md
- Use `docker compose ps -a -q` for the openfga-migrate lookup before `docker wait` in both the LitCal local task and the production task — plain `ps -q` misses an already-exited one-shot (reproduced empirically). - Add a no-op assertion to the production migration step: grep the captured logs for the pre-migration `"current version": 6` line, the only signal that actually distinguishes a no-op from an applied migration (verified against the v1.15.1 image — the "running all migrations"/"migration done" lines print identically either way). - Fix Step 10 of the model-ownership plan: it exercises the no-lock refusal branch, not "adoption" as previously labeled: validate the out-of-band setup upload fails loudly instead of silently producing an empty ID. - Stop discarding the fetched model ID in the production lock-verification loop; compare it against the expected ID alongside the content diff. - Harden the Task 7 live-vs-lock check: `jq -er '// empty'` instead of `-r` (a missing field printed literal "null" and could false-positive against another failure), plus an explicit HTTP status check.
#25 landed a one-line refinement to auth/handoffs/liturgicalcalendar.md — that consumers pinned to an earlier model ID still resolve `deleter` and `test_definition`. This branch rewrites that whole paragraph and deletes the "⚠ the copy in this repo is stale" section around it, so the two collide. Resolved in favour of this branch, which already carries that correction (and adds why it holds: the older model still exists in the store's history). The stale-copy warning stays deleted — it described a hazard this branch removes, by syncing the model and adding a guard that refuses to upload over a model this repo did not put there. No other file conflicted; main's newer plan docs are taken as-is.
The local stack those tasks were waiting on now exists, so the stale notice no longer describes reality.
Mergeable independently of the two application PRs — it documents them but does not depend on them.
What changed
docs/superpowers/plans/2026-08-03-martyrology-oidc-login-client.md:gh issue close --commentstill asserted two apps in one project; Step 5 was copy-pasteable straight into a public issue with the wrong claim.What did not change
The Global Constraints note — no localhost client in production Zitadel — is untouched and remains true. Task 1's original-code block and its already-executed commit message are left as historical record.
Enabled by
--provision-martyrology-frontendselect its origin by--target.CatholicOS/martyrology-api#29andCatholicOS/martyrology-frontend#16, whose design doc is cited as the description of record.Every factual claim was verified against the actual repo state rather than drafted from memory — the
case "$TARGET"block, all six cited step numbers, and the referenced file paths.One known staleness remains, deliberately: the note says the stack is on unmerged branches. That self-corrects when those PRs land, and is better than implying availability now.
🤖 Generated with Claude Code
Summary by CodeRabbit